Skip to content

test: cover v2/main mid-session-auth code to the ≥90% per-file gate#1611

Open
cliffhall wants to merge 4 commits into
v2/mainfrom
1610-midsession-coverage
Open

test: cover v2/main mid-session-auth code to the ≥90% per-file gate#1611
cliffhall wants to merge 4 commits into
v2/mainfrom
1610-midsession-coverage

Conversation

@cliffhall

@cliffhall cliffhall commented Jul 4, 2026

Copy link
Copy Markdown
Member

Closes #1610

What

Bring v2/main's mid-session / step-up auth code up to the ≥90% per-file coverage gate (all four dimensions). This debt is invisible on v2/main (its CI does not run npm run coverage) but the Wave-1 rollup (#1601, which includes the #1550 CI gate) surfaces it — so we fix it at the source here, then re-merge v2/main into the rollup.

Every failing file was reproduced on a clean v2/main checkout; none of this was introduced by the rollup.

Result — all three files now clear the gate

File Before After
clients/cli/src/cliOAuth.ts 75 / 78.57 / 75 / 75 100 / 96.42 / 100 / 100
clients/tui/src/components/AuthTab.tsx 88.46 / 86.73 / 93.33 / 91.78 97.43 / 90.81 / 100 / 100
clients/tui/src/App.tsx 79.01 / 82.51 / 85.71 / 79.73 92.23 / 90.20 / 92.85 / 92.68

(stmts / branch / funcs / lines)

How

  • cliOAuth.ts — cover confirmStepUpFromStdin (default stdin confirmer, via mocked node:readline/promises) and the AuthRecoveryRequiredError / isUnauthorizedError branches of connectInspectorWithOAuth.
  • AuthTab.tsx — cover the step-up useInput up-arrow, a/c shortcuts, and unrelated-key fall-through.
  • App.tsx — exercise the mid-session auth machinery (runOAuthAuthentication result branches, the per-client auth-lifecycle listeners, handleAuthRecoveryRequired, handleConnect's OAuth-on-401 branches, handleClearOAuth, and the AuthTab onAuthorizeStepUp handler). Two small, opt-in additions to the App test harness — a client-event registry on the FakeClient and a runRunnerInteractiveOAuth override that defaults to the real runner — keep every existing callback-driven OAuth test unchanged.

Verification

npm run validate and npm run coverage both pass from the repo root (web / cli / tui / launcher all green).

Follow-up

Once merged, v2/main is re-merged into 1579-wave-1 (#1601) so the rollup's coverage gate goes green.

🤖 Generated with Claude Code

https://claude.ai/code/session_01S3fTN8H3R8YV4yUGvZjYnX

cliffhall and others added 2 commits July 4, 2026 16:47
Bring clients/cli/src/cliOAuth.ts from 75% to ≥90% on all four coverage
dimensions (now 100/96.42/100/100) by exercising the previously-untested
interactive step-up and auth-recovery paths:

- confirmStepUpFromStdin (the default stdin confirmer): drive it via the
  default `confirmStepUp` parameter of handleCliAuthRecoveryRequired with
  node:readline/promises mocked — covers the y / "yes" (trimmed,
  case-insensitive) accept paths and the n decline path (which throws).
- connectInspectorWithOAuth's AuthRecoveryRequiredError branch: both the
  storage-already-satisfies resume and the interactive-recovery sub-branches,
  plus the isUnauthorizedError branch (with a rejecting disconnect to hit the
  `.catch(() => {})` guard), the non-OAuth-capable-config rethrow, and the
  non-OAuth-error rethrow.

Part of #1610 — v2/main mid-session-auth code below the per-file coverage
gate that v2/main's CI does not run but the Wave-1 rollup (#1550) does.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01S3fTN8H3R8YV4yUGvZjYnX
Bring clients/tui/src/components/AuthTab.tsx to ≥90 on all four dimensions
(now 97.43/90.81/100/100) by exercising the previously-untested step-up
keypress paths in the `useInput` handler: up-arrow navigation back to the
"authorize" choice, the `a` (authorize) and `c` (cancel) shortcuts, and the
fall-through for an unrelated key while the prompt is pending.

Part of #1610.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01S3fTN8H3R8YV4yUGvZjYnX
@cliffhall cliffhall added the v2 Issues and PRs for v2 label Jul 4, 2026
Bring clients/tui/src/App.tsx from 79% to ≥90 on all four dimensions
(92.23 / 90.20 / 92.85 / 92.68) by exercising the mid-session / step-up auth
machinery that the existing suite didn't reach:

- runOAuthAuthentication result branches (success / already_authorized /
  insufficient_scope / unsupported), driven deterministically via a new opt-in
  runRunnerInteractiveOAuth override (defaults to the real runner so the
  existing callback-driven OAuth tests are unchanged).
- The per-client auth-lifecycle listeners (authChallengeAmbient / Recovered /
  Interactive / oauthError), fired through a new client-event registry on the
  FakeClient, including the non-selected-server early-return guards.
- handleAuthRecoveryRequired step-up-confirm and reauth paths, handleConnect's
  OAuth-on-401 branches (AuthRecoveryRequired + EmaClientNotConfigured thrown
  mid-flow), handleClearOAuth's connected/disconnect path, and the AuthTab
  onAuthorizeStepUp handler (EMA satisfied/interactive/failed/throw + standard
  non-EMA authorize, cancel).

Part of #1610.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01S3fTN8H3R8YV4yUGvZjYnX
@cliffhall cliffhall marked this pull request as ready for review July 4, 2026 21:31
@cliffhall

Copy link
Copy Markdown
Member Author

@claude please review this PR for correctness and test quality. It adds coverage tests to bring three v2/main mid-session-auth files (clients/cli/src/cliOAuth.ts, clients/tui/src/components/AuthTab.tsx, clients/tui/src/App.tsx) up to the ≥90% per-file gate. Focus on: (1) are the new tests actually asserting real behavior (not just executing lines for coverage)? (2) are the two App.test.tsx harness additions (the client-event registry and the runRunnerInteractiveOAuth override that defaults to the real runner) sound and non-fragile? (3) any missed edge cases or incorrect assertions. If you find nothing substantive, say so explicitly.

@claude

claude Bot commented Jul 4, 2026

Copy link
Copy Markdown
Contributor

Claude finished @cliffhall's task in 4m 44s —— View job


Review — mid-session-auth coverage tests

I read the three source files under test (cliOAuth.ts, AuthTab.tsx, App.tsx) and the full diff of all three test files against origin/v2/main, and traced the message strings / helpers the assertions depend on.

Verdict: solid, mergeable. The tests assert real behavior, and the two harness additions are sound. One minor test-fidelity observation, no blockers.

Detailed findings

(1) Do the tests assert real behavior?

Yes. They are behavioral, not line-chasing:

  • cliOAuth.test.ts — the new confirmStepUpFromStdin tests drive the actual normalization logic ("y", whitespace-padded " YES " → proceed; "n" → throws "Step-up authorization declined.") and assert the finally { rl.close() } runs (mockClose called). The connectInspectorWithOAuth block distinguishes the three real branches: AuthRecoveryRequiredError + satisfied storage (reconnect, no interactive), unsatisfied (interactive runs), plain 401 (isUnauthorizedError matches "…failed…(401)" — confirmed against core/auth/utils.ts), non-OAuth rethrow, and non-OAuth-capable rethrow. The .catch(() => {}) disconnect guard is exercised with a rejecting disconnect. Assertions are on thrown messages and spy call counts — meaningful.
  • AuthTab.test.tsx — the up-arrow test writes DOWNUPENTER and asserts onAuthorizeStepUp (not onCancelStepUp) fires; if UP were a no-op the selection would sit at index 1 and Enter would cancel, so the assertion genuinely pins the Math.max(0, i-1) behavior. The a/c/x test asserts the unrelated key is swallowed (counts unchanged after x), covering the return; fall-through.
  • App.test.tsx — every mid-session test asserts a concrete outcome tied to a real message string, all of which I verified in source: "Refreshing authorization…" (App.tsx:843), "needs additional OAuth scopes" / "…organization before it can continue" (core/auth/oauthUx.ts:99-100, correctly EMA vs non-EMA per catalog), "were not granted" (oauthUx.ts:172-180), "enterprise…" (EmaClientNotConfiguredError, both reasons contain it), and the success/already_authorized/insufficient_scope/unsupported/skipped result branches of runOAuthAuthentication and the onAuthorizeStepUp EMA path (satisfied/interactive/failed/throws).

(2) Are the two harness additions sound / non-fragile?

Yes, both.

  • Client-event registryaddEventListener/removeEventListener now maintain a Map<event, Set<fn>>; App's per-client cleanup removes exactly the fns it added, and beforeEach calls clientEvents.clear(), so there's no cross-test leak. React effects flush after render (not during dispatch), so forEach isn't mutated mid-iteration. Fine.
  • runRunnerInteractiveOAuth override defaulting to the real runner — the wrapper falls through to actual.runRunnerInteractiveOAuth when override is null, so the pre-existing callback-driven tests (which drive h.cb.opts.onCallback) keep working untouched; override is reset in beforeEach. When set, it returns a deterministic { kind }. A never-resolving default runner in a fire-and-forget void (async …)() flow doesn't hang the test because the assertions poll intermediate state (callbackStart called) rather than awaiting completion. Sound.

(3) Missed edge cases / observations (minor, non-blocking)

  1. "ignores auth lifecycle events from a non-selected server" is a coverage test, not a true guard test. Because the registry is keyed by event name only, fireClientEvent("oauthError", …) fires both the selected (web) and non-selected (api) handlers. Both produce the same observable side effect (web sets the message; api early-returns), so the assertion ("selected-only error" appears) would still pass even if the selectedServerRef.current !== serverName guard were removed. It exercises the early-return branch for coverage but doesn't pin its behavior. Truly asserting the guard would require firing an event for only the non-selected client, which the event-name-keyed registry can't express. Consider this a known fidelity gap rather than a fix-now item.

  2. A couple of negative assertions rely on fixed ticks (e.g. await tick(); await tick(); expect(h.connect).toHaveBeenCalledTimes(1) in the "unsupported OAuth result" test). These are acceptable since the override resolves immediately and a wrongful reconnect would land within two ticks, but they're inherently the most timing-sensitive assertions in the suite if the flow ever grows an extra await.

  3. runOAuthAuthentication's "skipped" (oauthInProgressRef already true → "OAuth already in progress.") branches don't appear to be directly exercised — consistent with the reported branch % staying below 100 (cli 96.4, App 90.2). Above the gate, so fine to leave.

Nothing here blocks merge — the suite tests real behavior and the harness changes are clean and reset per-test. The only thing I'd flag for future hardening is observation (1): the non-selected-server guard is covered but not behaviorally asserted.
· branch 1610-midsession-coverage

Addresses the @claude review of #1611: the event registry is now keyed by
(client, fn) rather than event name alone, and a new `fireClientEventFor(client,
…)` helper fires a single client's listeners. The "ignores auth lifecycle events
from a non-selected server" test now fires ambient/recovered/oauthError for ONLY
the non-selected client and asserts no message appears — so removing the
`selectedServerRef.current !== serverName` guard would fail the test — then fires
for the selected client to confirm it does act. Coverage of App.tsx is unchanged
(92.23 / 90.20 / 92.85 / 92.68); existing fire-all `fireClientEvent` behavior is
preserved.

Part of #1610.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01S3fTN8H3R8YV4yUGvZjYnX
@cliffhall

Copy link
Copy Markdown
Member Author

@claude thanks — addressed observation (1) in 1f7ef918: the App.test.tsx event registry is now keyed by (client, fn) with a new fireClientEventFor(client, …) helper, and the "ignores auth lifecycle events from a non-selected server" test now fires ambient/recovered/oauthError for only the non-selected client and asserts no message appears (so removing the selectedServerRef.current !== serverName guard would fail the test), then fires for the selected client to confirm it acts. App.tsx coverage is unchanged (92.23 / 90.20 / 92.85 / 92.68) and the existing fire-all fireClientEvent behavior is preserved.

On (2) and (3): left as-is intentionally — the fixed-tick negative assertions rely on the override resolving synchronously (a wrongful reconnect lands within the polled window), and the oauthInProgress → "skipped" branches would need a contrived concurrent-OAuth race to hit; all files clear the ≥90 gate without them. Please re-review at head 1f7ef918 and confirm whether any complaints remain.

@claude

claude Bot commented Jul 4, 2026

Copy link
Copy Markdown
Contributor

Claude finished @cliffhall's task in 1m 54s —— View job


Re-review at 1f7ef918 — observation (1) fix

I read the updated harness + test and the guarded listeners in clients/tui/src/App.tsx:839-874, then mutation-tested the fix to confirm the guard is genuinely pinned (not just executed).

Verdict: the fix does what it claims. No remaining complaints — mergeable.

Detailed findings

The registry rekey is sound

clientEvents is now Map<event, Set<{client, fn}>>, addEventListener records the owning FakeClient, and removeEventListener matches by fn (each client builds its own onAmbient/onOAuthError closures, so fn is unique per client — the by-fn delete is unambiguous). clientInstances is reset in beforeEach (h.clientInstances.length = 0) alongside clientEvents.clear(), so no cross-test leak. fireClientEvent still fires every client's handler, preserving all pre-existing single-server tests. Clean.

The guard is now behaviorally asserted, not just covered

The test resolves api/web by their url (http://b / http://a from twoHttp()), fires ambient/recovered/oauthError for only api (non-selected), and asserts the frame contains neither "Refreshing authorization" nor "api-only error", then fires web and asserts both surface. This closes the exact fidelity gap I flagged: with the event-name-only registry the old test would have passed even with the guard deleted.

I verified that empirically — temporarily removing the selectedServerRef.current !== serverName guard on onAmbient (App.tsx:841) fails the test:

AssertionError: expected '…' not to contain 'Refreshing authorization'
❯ __tests__/App.test.tsx:1451

Restored after. Test passes clean at head (1 passed / 79 skipped). So the ambient and oauthError guards are now true regression guards.

One residual, genuinely-minor nuance (non-blocking)

Of the three fired events, only ambient and oauthError are behaviorally pinned. The onRecovered guard (App.tsx:846) has no distinguishable observable effect when it runs — it does setOauthMessage(null) (message is already null here) + bumps oauthRevision — so removing its guard alone wouldn't flip any assertion. That's a limitation of the handler having no visible side effect to observe, not of the test design; the two guards that can produce output are both pinned. Not worth contriving state to chase.

(2) and (3) left as-is — agreed

The fixed-tick negative assertions are fine given the override resolves synchronously, and the oauthInProgress → "skipped" branch would need a contrived concurrent-OAuth race; all three files clear the ≥90 gate without it. Reasonable to leave.

Nice work — the harness rekey is minimal and the guard is now a real regression test (mutation-verified). Ship it.
· branch 1610-midsession-coverage

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

v2 Issues and PRs for v2

Projects

None yet

Development

Successfully merging this pull request may close these issues.

test: cover v2/main mid-session-auth code to the ≥90% per-file gate (cliOAuth.ts, TUI App.tsx, AuthTab.tsx)

1 participant